Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed Sensitive Data Exposure at /tmp directory #5375

Merged
merged 1 commit into from
Jan 10, 2024
Merged

Fixed Sensitive Data Exposure at /tmp directory #5375

merged 1 commit into from
Jan 10, 2024

Conversation

fazledyn-or
Copy link
Contributor

Summary

This PR fixes a case of sensitive data exposure by using mkstemp method to create dbus.log file and a custom opener to create anaconda-tb-all.log.

Description

While triaging your project, our bug fixing tool generated the following message(s)-

In file: dbus_launcher.py, there is a method that creates a temporary file using a hard coded path to a public writable directory (CWE-379). This exposes the temporarily created file to race condition attacks. An attacker can access, modify or even delete a file. iCR suggested that a temporary file should be created using a safe API in a secured directory instead.

In file: anaconda.py, there is a method that creates a temporary file using a hard coded path to a public writable directory (CWE-379). This exposes the temporarily created file to race condition attacks. An attacker can access, modify or even delete a file. iCR suggested that a temporary file should be created using a safe API in a secured directory instead.

There were two cases which required individual fixes for each of them. I've updated them as it seemed correct to me. Please have a look and let me know what you think. If required, I can update the PR accordingly.

Previously Found & Fixed

Below is a list of open-source projects where this same bug was found and fixed-

CLA Requirements

This section is only relevant if your project requires contributors to sign a Contributor License Agreement (CLA) for external contributions.

All contributed commits are already automatically signed off.

The meaning of a signoff depends on the project, but it typically certifies that committer has the rights to submit this work under the same license and agrees to a Developer Certificate of Origin (see https://developercertificate.org/ for more information).

- Git Commit SignOff documentation

Sponsorship and Support

This work is done by the security researchers from OpenRefactory and is supported by the Open Source Security Foundation (OpenSSF): Project Alpha-Omega. Alpha-Omega is a project partnering with open source software project maintainers to systematically find new, as-yet-undiscovered vulnerabilities in open source code - and get them fixed – to improve global software supply chain security.

The bug is found by running the Intelligent Code Repair (iCR) tool by OpenRefactory and then manually triaging the results.

@github-actions github-actions bot added the f40 label Dec 12, 2023
KKoukiou
KKoukiou requested changes Dec 15, 2023
View reviewed changes
pyanaconda/anaconda.py Outdated Show resolved Hide resolved
KKoukiou
KKoukiou previously approved these changes Jan 3, 2024
View reviewed changes
KKoukiou
KKoukiou reviewed Jan 3, 2024
View reviewed changes
Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine I just would prefer the commits squashed, as the change from the second commit is introduced in the first commit.

@fazledyn-or fazledyn-or force-pushed the Fix_Sensitive_Data_Exposure_tmp branch from 639f754 to 2006c74 Compare January 3, 2024 15:22
@fazledyn-or
Copy link
Contributor Author

This is fine I just would prefer the commits squashed, as the change from the second commit is introduced in the first commit.

Done. Can you please check now?

@KKoukiou KKoukiou mentioned this pull request Jan 3, 2024
Merged
KKoukiou
KKoukiou reviewed Jan 3, 2024
View reviewed changes
Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that we hardcode this dbus.log in more places. See pyanaconda/exception.py

We need to match the file matching the pattern there and include it to the file list.

@KKoukiou KKoukiou dismissed their stale review January 3, 2024 15:55

Needs a bit more work.

KKoukiou
KKoukiou reviewed Jan 3, 2024
View reviewed changes
Copy link
Contributor

@KKoukiou KKoukiou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly pyanaconda/modules/boss/installation.py and tests/unit_tests/pyanaconda_tests/modules/boss/test_copy_logs_task.py can't have hardcoded the file name anymore.

@fazledyn-or
Copy link
Contributor Author

Similarly pyanaconda/modules/boss/installation.py and tests/unit_tests/pyanaconda_tests/modules/boss/test_copy_logs_task.py can't have hardcoded the file name anymore.

How about we use open_with_perm for the dbus.log too?

@KKoukiou
Copy link
Contributor

KKoukiou commented Jan 4, 2024

Similarly pyanaconda/modules/boss/installation.py and tests/unit_tests/pyanaconda_tests/modules/boss/test_copy_logs_task.py can't have hardcoded the file name anymore.

How about we use open_with_perm for the dbus.log too?

Sounds like the easy way to go forward indeed!

@fazledyn-or fazledyn-or force-pushed the Fix_Sensitive_Data_Exposure_tmp branch from 2006c74 to 06bcdcf Compare January 8, 2024 04:43
@fazledyn-or
Copy link
Contributor Author

Similarly pyanaconda/modules/boss/installation.py and tests/unit_tests/pyanaconda_tests/modules/boss/test_copy_logs_task.py can't have hardcoded the file name anymore.

How about we use open_with_perm for the dbus.log too?

Sounds like the easy way to go forward indeed!

Please have a look.

@fazledyn-or
Fixed file-write operation to a public directory
9cb6f74 
This PR fixes a case of sensitive data exposure by
using the existing helped method `open_with_perm`.

Signed-off-by: fazledyn-or <[email protected]>
@fazledyn-or fazledyn-or force-pushed the Fix_Sensitive_Data_Exposure_tmp branch from 06bcdcf to 9cb6f74 Compare January 8, 2024 04:45
@KKoukiou
Copy link
Contributor

/kickstart-test --testtype smoke

@KKoukiou KKoukiou merged commit 3d1e979 into rhinstaller:master Jan 10, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KKoukiou KKoukiou KKoukiou approved these changes

Assignees
No one assigned
Labels
f40
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@fazledyn-or @KKoukiou